Skip to content

Conversation

@jeremylong
Copy link
Contributor

resolves #100

This should only be merged after #476.

@chadlwilson
Copy link
Collaborator

chadlwilson commented Oct 13, 2025

Honestly, this is a bit scary. Can you please hold off?

  1. The outputDirectory it uses by default is not one that is exclusively owned by the plugin; it is overlapping with the standard directories gradle ReportingExtension plugins use (layout.buildDirectory.dir('reports') including Gradle's own problem report.

    Caching disabled for task ':dependencyCheckAggregate' because:
      Gradle does not know how file 'target/reports/tests' was created (output property 'outputDir'). Task output caching requires exclusive access to output paths to guarantee correctness (i.e. multiple tasks are not allowed to produce output in the same location).
    
    $ tree target/reports | head -10
    target/reports
    ├── dependency-check-report.html
    ├── problems
    │   └── problems-report.html
    └── tests
        └── allTests
            ├── classes
            │   ├── com.thoughtworks.go.agent.AgentControllerTest.html
    

    The default configuration of the plugin should not clash with standard locations by default (or it should define only OutputFiles inside reports rather than the entire dir). The caching of outputs isn't a big deal as other plugins aren't so likely to depend on dep check's output, however that's not necesarily true.

  2. There is also a big risk of ending up with stale results if you do this stuff incorrectly. In particular, the way this plugin resolves configurations and depends on other projects is unsafe, and may lead to it not re-running with another proejct's configuration is changed.

    e.g in my project I changed a dependency in a configuration for a subproject and re-ran.

    > Task :dependencyCheckAggregate UP-TO-DATE
    Custom actions are attached to task ':dependencyCheckAggregate'.
    Caching disabled for task ':dependencyCheckAggregate' because:
      Gradle does not know how file 'target/reports/tests' was created (output property 'outputDir'). Task output caching requires exclusive access to output paths to guarantee correctness (i.e. multiple tasks are not allowed to produce output in the same location).
    Skipping task ':dependencyCheckAggregate' as it is up-to-date.
    

    This is incorrect, dep check MUST re-run when relevant non-skipped input configurations have changed - however it does not model its inputs or dependent projects correctly.

  3. Generally speaking the plugin probably needs to re-run (not have results cached) anyway, because it needs to check remote data sources such as NVD etc which may have changed since the last run. This isn't the type of plugin that suits the use of task caching unless you do something really sophisticated to link the cache expiries of all the analyzers.

@chadlwilson
Copy link
Collaborator

I think the OP in #100 (comment) is highlighting what I mention in #3) by suggesting that the "update" bits would need to be factored out into a separate task which can basically run every time to check NVD/OssIndex etc.

If those don't lead to a change in the dataDirectory contents, and there are no changed configurations, or files matching the scanSet, then theoretically you dont need to re-run the analyze.

Unfortunately, the way the engine is set up and the way many analyzers are "online" (not cacheable), I think it probably makes it a bit difficult to do this task/build caching in a predictable manner and to be useful?

Conceptually, you'd want every analyzer to be marked as to whether it depends on a "mutable online source" and then for the up-to-date-ness of the Gradle tasks to depend on the analyzers enabled.

@jeremylong
Copy link
Contributor Author

This PR and #474 are not going to be merged without a lot more review, testing, and consideration of consequences.

Regarding point 3 above - I understand the need/desire to re-run the task if a day or more has passed and nothing has changed. However, if I run a build back-to-back that invokes the task, I believe it is fair to expect the task to be skipped if none of the inputs or outputs have been changed.

@chadlwilson
Copy link
Collaborator

chadlwilson commented Oct 13, 2025

Ok, cool - I thouhht perhaps you wanted to get it into the fix release today 😅

But yes I agree it'd be nice to define some semantics on up-to-date-ness here that are cache-aware (for NVD) and allow the user to, say, opt-out of rechecking certain online sources more frequently than X minutes even if they don't support caching of their results

@chadlwilson
Copy link
Collaborator

I guess you could try and make this dynamic based on information using custom https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/TaskOutputs.html if the engine configuration can answer some questions somehow.

Like in pseudo-code:

outputs.cacheIf("All enabled analyzers are cacheable") { task ->
  task.forceCachingOfOnlineAnalyzers.get() || task.engine.allEnabledAnalyzersAreCacheable()
}

outputs.upToDateWhen { task ->
  !task.engine.needsDataSourceCacheRefresh()
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tasks not cached

3 participants